Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a lightweight Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as Application Startup
participant RPC as RPC Endpoint
participant State as AppState
participant DB as Database
Startup->>RPC: fetch_chain_id (eth_chainId)
RPC-->>Startup: chain ID (hex)
Startup->>Startup: parse_chain_id(hex)
Startup->>State: initialize state with chain_id, chain_name
Startup->>DB: establish DB connections
participant Client as StatusPage (Browser)
participant API as Atlas API
Client->>API: GET /api/status (getChainStatus)
API->>State: read chain_id, chain_name
API->>DB: query block_height, total_transactions, total_addresses
API-->>Client: ChainStatusResponse
Client->>Client: render status, start polling loop
loop Every 5s
Client->>API: GET /api/height (fast poll)
API->>DB: read last_indexed_block
API-->>Client: HeightResponse {block_height, indexed_at}
Client->>Client: update UI block height
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6fcae3a to
8df58c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/crates/atlas-api/src/handlers/status.rs (1)
27-63:⚠️ Potential issue | 🟠 MajorAdd same-file unit tests for the new status handler logic.
This file adds new response shapes and query flow but does not include a
#[cfg(test)] mod testsblock here.As per coding guidelines "Add unit tests for new logic in a
#[cfg(test)] mod testsblock in the same file. Run all tests withcargo test --workspace."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/atlas-api/src/handlers/status.rs` around lines 27 - 63, Add a #[cfg(test)] mod tests block to this file containing unit tests for get_height and get_status: mock or construct an Arc<AppState> with a test sqlx::Sqlite/Postgres in-memory pool (or use sqlx::query mocking), insert a row for key 'last_indexed_block', and assert that get_height returns the expected HeightResponse and get_status returns ChainStatus with correct block_height, indexed_at, total_transactions and total_addresses (exercise get_table_count by creating test rows for "transactions" and "addresses" or mocking it). Name tests clearly (e.g., test_get_height_returns_parsed_height, test_get_status_returns_full_chain_status), import necessary types (AppState, HeightResponse, ChainStatus, get_height, get_status, get_table_count) and run them with cargo test --workspace to ensure compilation and behavior.
🧹 Nitpick comments (1)
backend/crates/atlas-api/src/handlers/mod.rs (1)
55-58: Consider identifier quoting for defense-in-depth.The
format!macro directly interpolatestable_nameinto the SQL query. While all current callers pass hardcoded string literals (verified instatus.rsandtransactions.rs), adding identifier quoting would provide defense-in-depth against future misuse.Optional: Quote the identifier
// Exact count for small tables - let exact: (i64,) = sqlx::query_as(&format!("SELECT COUNT(*) FROM {table_name}")) + let exact: (i64,) = sqlx::query_as(&format!("SELECT COUNT(*) FROM \"{}\"", table_name)) .fetch_one(pool) .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/atlas-api/src/handlers/mod.rs` around lines 55 - 58, The query interpolates table_name directly into sql and should quote and escape the identifier for defense-in-depth: when building the sql for sqlx::query_as (where variable table_name is used to produce exact), wrap the identifier in double quotes and escape any embedded double quotes by replacing " with "" before interpolation (or use sqlx's QueryBuilder/identifier-quoting helper if available) so the final string becomes a safely quoted table identifier instead of raw table_name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 4-6: The CHAIN_NAME value contains spaces and should be quoted;
update the .env.example entry for CHAIN_NAME to surround the value with double
quotes (e.g., CHAIN_NAME="My Chain") so dotenv-compatible loaders parse it
reliably and consistently.
In `@backend/crates/atlas-api/src/handlers/status.rs`:
- Around line 25-43: The /api/status contract was changed incorrectly — restore
get_status to be the lightweight endpoint that performs a single key-value
lookup against the indexer_state table and returns { block_height, indexed_at }
(the same shape as HeightResponse) and remove or rename the separate full-info
endpoint; specifically, move the SQL query "SELECT value, updated_at FROM
indexer_state WHERE key = 'last_indexed_block'" and its parsing logic (currently
in get_height) into get_status, have get_status return Json<HeightResponse>
using result.0.parse() for block_height and result.1.to_rfc3339() for
indexed_at, and delete or non-routify the get_height function/route so there is
only one /api/status endpoint complying with the guideline.
In `@frontend/src/App.tsx`:
- Around line 38-43: Routes for the "tokens" and "tokens/:address" entries have
inconsistent indentation compared to the other <Route> siblings; align the JSX
so all <Route ... /> elements in App.tsx use the same indentation level (e.g.,
match the indentation used by <Route path="nfts" ... />, <Route path="status"
... /> and others). Edit the lines containing the <Route path="tokens"
element={<TokensPage />} /> and <Route path="tokens/:address"
element={<TokenDetailPage />} /> entries to remove the extra leading space and
ensure consistent indentation across the Route list.
---
Outside diff comments:
In `@backend/crates/atlas-api/src/handlers/status.rs`:
- Around line 27-63: Add a #[cfg(test)] mod tests block to this file containing
unit tests for get_height and get_status: mock or construct an Arc<AppState>
with a test sqlx::Sqlite/Postgres in-memory pool (or use sqlx::query mocking),
insert a row for key 'last_indexed_block', and assert that get_height returns
the expected HeightResponse and get_status returns ChainStatus with correct
block_height, indexed_at, total_transactions and total_addresses (exercise
get_table_count by creating test rows for "transactions" and "addresses" or
mocking it). Name tests clearly (e.g., test_get_height_returns_parsed_height,
test_get_status_returns_full_chain_status), import necessary types (AppState,
HeightResponse, ChainStatus, get_height, get_status, get_table_count) and run
them with cargo test --workspace to ensure compilation and behavior.
---
Nitpick comments:
In `@backend/crates/atlas-api/src/handlers/mod.rs`:
- Around line 55-58: The query interpolates table_name directly into sql and
should quote and escape the identifier for defense-in-depth: when building the
sql for sqlx::query_as (where variable table_name is used to produce exact),
wrap the identifier in double quotes and escape any embedded double quotes by
replacing " with "" before interpolation (or use sqlx's
QueryBuilder/identifier-quoting helper if available) so the final string becomes
a safely quoted table identifier instead of raw table_name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a6ab6a6-fbcc-4885-b4ae-d1effce5f286
📒 Files selected for processing (12)
.env.exampleCLAUDE.mdbackend/crates/atlas-api/src/handlers/mod.rsbackend/crates/atlas-api/src/handlers/status.rsbackend/crates/atlas-api/src/handlers/transactions.rsbackend/crates/atlas-api/src/main.rsdocker-compose.ymlfrontend/src/App.tsxfrontend/src/api/status.tsfrontend/src/components/Layout.tsxfrontend/src/pages/StatusPage.tsxfrontend/src/pages/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/crates/atlas-api/src/main.rs (2)
292-340: Good test coverage addition; add one more case for missingresult.You covered success, invalid hex, and transport failure. Add a test for the Line 38-40 path where
resultis absent/null to fully cover error branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/atlas-api/src/main.rs` around lines 292 - 340, Add a new async test in the tests module that uses serve_json_once to return a JSON-RPC response missing the "result" field (e.g. {"jsonrpc":"2.0","id":1}) and assert that fetch_chain_id(&url).await returns an error containing the expected message for a missing/null result; name it something like fetch_chain_id_returns_error_for_missing_result to match the existing test style and ensure it covers the code path handling absent result in fetch_chain_id.
80-84: Add bounded retry for startup chain ID fetch.Line 82 currently fails startup on the first transient RPC error. A short retry/backoff loop would improve boot reliability without changing fail-closed behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/atlas-api/src/main.rs` around lines 80 - 84, Wrap the single call to fetch_chain_id(&rpc_url).await? in a bounded retry/backoff loop so transient RPC errors don't fail startup immediately: perform N attempts (e.g. 3-5) with an increasing delay (e.g. exponential backoff using tokio::time::sleep) between attempts, log each retry via tracing::info!/error! including attempt count and error, and return the original error only if all attempts are exhausted; update the startup block in main.rs where rpc_url and fetch_chain_id are used to implement this retry logic around fetch_chain_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/crates/atlas-api/src/main.rs`:
- Around line 263-264: Remove the duplicate status surface by deleting the
.route("/api/height", get(handlers::status::get_height)) registration and ensure
the single chain-status handler handlers::status::get_status is the only exposed
endpoint; update handlers::status::get_status (and any related types) to return
exactly the contract shape { block_height, indexed_at } so clients rely solely
on GET /api/status for chain status information.
---
Nitpick comments:
In `@backend/crates/atlas-api/src/main.rs`:
- Around line 292-340: Add a new async test in the tests module that uses
serve_json_once to return a JSON-RPC response missing the "result" field (e.g.
{"jsonrpc":"2.0","id":1}) and assert that fetch_chain_id(&url).await returns an
error containing the expected message for a missing/null result; name it
something like fetch_chain_id_returns_error_for_missing_result to match the
existing test style and ensure it covers the code path handling absent result in
fetch_chain_id.
- Around line 80-84: Wrap the single call to fetch_chain_id(&rpc_url).await? in
a bounded retry/backoff loop so transient RPC errors don't fail startup
immediately: perform N attempts (e.g. 3-5) with an increasing delay (e.g.
exponential backoff using tokio::time::sleep) between attempts, log each retry
via tracing::info!/error! including attempt count and error, and return the
original error only if all attempts are exhausted; update the startup block in
main.rs where rpc_url and fetch_chain_id are used to implement this retry logic
around fetch_chain_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7471d202-a2de-49c1-ab17-523418b305c0
📒 Files selected for processing (3)
.env.examplebackend/crates/atlas-api/src/main.rsfrontend/src/App.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.example
- frontend/src/App.tsx
| /// Returns in <1ms, optimized for frequent polling. | ||
| pub async fn get_height(State(state): State<Arc<AppState>>) -> ApiResult<Json<HeightResponse>> { | ||
| let result: (String, chrono::DateTime<chrono::Utc>) = sqlx::query_as( | ||
| "SELECT value, updated_at FROM indexer_state WHERE key = 'last_indexed_block'", |
There was a problem hiding this comment.
this is still blocked on items written to disk, if we batch write then its not really latest height but latest written height. what are your thoughts on doing a internal api on the indexer to talk to the api process to allow direct streaming before writing?
There was a problem hiding this comment.
I'll wait for the SSE refactoring to be merged and than we can read this value directly from the latest block in the buffer 👍
…ight - Rename existing status endpoint to /api/height (block_height + indexed_at) - Add new /api/status with chain_id, chain_name, total_transactions, total_addresses - Fetch chain_id once from RPC at startup and store in AppState - Read CHAIN_NAME from env, defaults to Unknown - Make get_table_count accept a table_name parameter, reused for addresses
- Add StatusPage showing chain_id, chain_name, block_height, total_transactions, total_addresses - Add getChainStatus() API call to /api/status - Update getStatus() to call /api/height - Add route and nav link for status page
15033ab to
783aaf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/crates/atlas-server/src/api/handlers/mod.rs (1)
20-57: 🛠️ Refactor suggestion | 🟠 MajorAdd in-file unit tests for the new counting logic.
This introduces new branching behavior and should include
#[cfg(test)] mod testscoverage in the same file (threshold path, parent fallback, and unsupported table handling if allowlisted).As per coding guidelines "
backend/**/*.rs: Add unit tests for new logic in a#[cfg(test)] mod testsblock in the same file, and run withcargo test --workspace."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/atlas-server/src/api/handlers/mod.rs` around lines 20 - 57, Add an in-file test module (#[cfg(test)] mod tests) under the same file that contains async unit tests for get_table_count: one test that seeds/mocks pg_class+pg_inherits to produce a large approx_partitions sum (>100_000) and asserts get_table_count returns the approx value (threshold path), one test that leaves no partition rows so the parent fallback path (query selecting reltuples in parent) is used and the returned count matches the parent reltuples, and one test for the "unsupported table handling if allowlisted" scenario described by the review (mock or seed state to represent the allowlist and assert the expected behavior). Locate tests next to get_table_count and reference the same symbols (get_table_count, approx_partitions, parent) and ensure tests run with async runtime and are executed by cargo test --workspace.
🧹 Nitpick comments (1)
backend/crates/atlas-server/src/api/handlers/status.rs (1)
34-34: Consider logging parse failures instead of silently defaulting to 0.Using
.unwrap_or(0)masks potential data corruption or schema issues inindexer_state.value. Ablock_heightof 0 could be mistaken for "no blocks indexed yet" rather than "parse failed."♻️ Proposed improvement
- let block_height: i64 = result.0.parse().unwrap_or(0); + let block_height: i64 = result.0.parse().unwrap_or_else(|e| { + tracing::warn!("Failed to parse block height '{}': {}", result.0, e); + 0 + });Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/atlas-server/src/api/handlers/status.rs` at line 34, The current parse of result.0 into block_height uses .unwrap_or(0), hiding parsing errors; replace it with explicit parse handling that logs the parse failure (including the offending string result.0 and the parse error) and only falls back to a sentinel or returns an error as appropriate; update the let block_height: i64 = result.0.parse().unwrap_or(0); line to use a match or Result-based flow that calls the request/handler logger (or error macro used in this module) to record the failure for indexer_state.value, and apply the same change to the other occurrence mentioned (around lines 50) so parsing problems aren’t silently turned into 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/crates/atlas-server/src/api/handlers/mod.rs`:
- Line 52: Replace the unsafe dynamic identifier interpolation used in the
sqlx::query_as call (format!("SELECT COUNT(*) FROM {table_name}")) by
whitelisting allowed table names with a match on the table_name variable and
mapping each branch to a hard-coded, prebuilt query string (or prepared
statement) so the code never interpolates identifiers at runtime; update the
code paths that set the exact: (i64,) result and any approximate-count logic to
use the chosen query string. Also add a #[cfg(test)] mod tests block in the same
file (mod.rs) containing unit tests that exercise both the approximate-count
branch (simulate reltuples > 100_000) and the exact-count fallback branch
(simulate reltuples < 100_000), asserting the expected behavior of the function
that uses sqlx::query_as and the exact variable; use test doubles or a test
database fixture as appropriate to isolate the two scenarios.
---
Outside diff comments:
In `@backend/crates/atlas-server/src/api/handlers/mod.rs`:
- Around line 20-57: Add an in-file test module (#[cfg(test)] mod tests) under
the same file that contains async unit tests for get_table_count: one test that
seeds/mocks pg_class+pg_inherits to produce a large approx_partitions sum
(>100_000) and asserts get_table_count returns the approx value (threshold
path), one test that leaves no partition rows so the parent fallback path (query
selecting reltuples in parent) is used and the returned count matches the parent
reltuples, and one test for the "unsupported table handling if allowlisted"
scenario described by the review (mock or seed state to represent the allowlist
and assert the expected behavior). Locate tests next to get_table_count and
reference the same symbols (get_table_count, approx_partitions, parent) and
ensure tests run with async runtime and are executed by cargo test --workspace.
---
Nitpick comments:
In `@backend/crates/atlas-server/src/api/handlers/status.rs`:
- Line 34: The current parse of result.0 into block_height uses .unwrap_or(0),
hiding parsing errors; replace it with explicit parse handling that logs the
parse failure (including the offending string result.0 and the parse error) and
only falls back to a sentinel or returns an error as appropriate; update the let
block_height: i64 = result.0.parse().unwrap_or(0); line to use a match or
Result-based flow that calls the request/handler logger (or error macro used in
this module) to record the failure for indexer_state.value, and apply the same
change to the other occurrence mentioned (around lines 50) so parsing problems
aren’t silently turned into 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cea1f2a7-5571-4daa-8506-630104f6b7e9
📒 Files selected for processing (14)
.env.exampleCLAUDE.mdbackend/crates/atlas-server/src/api/handlers/mod.rsbackend/crates/atlas-server/src/api/handlers/status.rsbackend/crates/atlas-server/src/api/handlers/transactions.rsbackend/crates/atlas-server/src/api/mod.rsbackend/crates/atlas-server/src/config.rsbackend/crates/atlas-server/src/main.rsdocker-compose.ymlfrontend/src/App.tsxfrontend/src/api/status.tsfrontend/src/components/Layout.tsxfrontend/src/pages/StatusPage.tsxfrontend/src/pages/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.example
- frontend/src/pages/StatusPage.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/crates/atlas-server/src/api/handlers/mod.rs`:
- Around line 19-21: Remove the redundant `.into()` on the formatted string
passed to the `sqlx::Error::Protocol` constructor: the `format!("unsupported
table for exact count: {table_name}")` already yields a `String`, so update the
`Err(sqlx::Error::Protocol(...))` arm (the branch that returns the unsupported
table error) to pass the `String` directly to `sqlx::Error::Protocol` (leave the
`table_name` interpolation and surrounding message unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3416c1f9-2bb6-4801-a1c4-eace15a19cb7
📒 Files selected for processing (1)
backend/crates/atlas-server/src/api/handlers/mod.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/crates/atlas-server/src/api/handlers/mod.rs (1)
34-69:⚠️ Potential issue | 🟠 MajorReject unsupported tables before the approximate-count branch.
Right now the whitelist only runs in the exact branch. An existing unsupported table above the threshold will return
Ok(approx)instead of erroring, soget_table_countchanges behavior based on row count. Validatetable_nameup front (or switch the parameter to an enum) so only the intended tables can use this helper.As per coding guidelines, `For large tables (transactions, addresses), use pg_class.reltuples for row count estimation instead of COUNT(*)`, with fallback to exact `COUNT(*)` for tables under 100k rows.🔧 Suggested fix
pub async fn get_table_count(pool: &PgPool, table_name: &str) -> Result<i64, sqlx::Error> { + let exact_sql = exact_count_sql(table_name)?; + // Sum approximate reltuples across partitions if any, else use parent. // This is instant and reasonably accurate for large tables. // Cast to float8 (f64) since reltuples is float4 and SUM returns float4 let approx_partitions: (Option<f64>,) = sqlx::query_as( r#" @@ if should_use_approximate_count(approx) { Ok(approx) } else { // Exact count for small tables - let exact: (i64,) = sqlx::query_as(exact_count_sql(table_name)?) + let exact: (i64,) = sqlx::query_as(exact_sql) .fetch_one(pool) .await?; Ok(exact.0) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/atlas-server/src/api/handlers/mod.rs` around lines 34 - 69, get_table_count currently only validates the allowed/unsupported table names in the exact-count branch, so large unsupported tables can return Ok(approx); move or add the whitelist validation at the start of get_table_count (or change the function to accept a validated enum) so table_name is rejected immediately before computing approx_partitions or calling should_use_approximate_count; specifically, ensure the same whitelist check used in the exact branch is invoked at the top of get_table_count (before calling approx_partitions query) and return an error for unsupported names so only intended tables (e.g., transactions, addresses) can use reltuples-based estimation with fallback to exact_count_sql when should_use_approximate_count decides to use COUNT(*).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/crates/atlas-server/src/api/handlers/mod.rs`:
- Around line 34-69: get_table_count currently only validates the
allowed/unsupported table names in the exact-count branch, so large unsupported
tables can return Ok(approx); move or add the whitelist validation at the start
of get_table_count (or change the function to accept a validated enum) so
table_name is rejected immediately before computing approx_partitions or calling
should_use_approximate_count; specifically, ensure the same whitelist check used
in the exact branch is invoked at the top of get_table_count (before calling
approx_partitions query) and return an error for unsupported names so only
intended tables (e.g., transactions, addresses) can use reltuples-based
estimation with fallback to exact_count_sql when should_use_approximate_count
decides to use COUNT(*).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf4ec5ca-bf79-4308-a4e5-f6e761f106b3
📒 Files selected for processing (1)
backend/crates/atlas-server/src/api/handlers/mod.rs
Overview
Adds a new /api/status endpoint returning chain ID, chain name, block height, total transactions, and total addresses, with chain name configurable via CHAIN_NAME env var. Renames the existing status endpoint to /api/height to better reflect its purpose, and adds a frontend status page displaying all chain information.
Summary by CodeRabbit
New Features
Chores